feat(oidc-client): add oidc session check with response type of id_token and none#682
feat(oidc-client): add oidc session check with response type of id_token and none#682vatsalparikh wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: 98f1e5c The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughAdds OIDC session status checking via a new ChangesSession Check Feature Implementation
Sequence DiagramssequenceDiagram
participant App
participant OidcClient as oidc() client
participant SessionMicro as sessionCheckNoneµ/IdTokenµ
participant StorageClient
participant IFrameManager
participant IFrameDispatch as sessionCheckIframe RTK
participant AuthorizationServer as IdP
App->>OidcClient: session.check({responseType: 'none'})
OidcClient->>SessionMicro: call with config + options
SessionMicro->>StorageClient: read stored idToken
StorageClient-->>SessionMicro: idToken (or null)
SessionMicro->>SessionMicro: build authorize URL with prompt=none
SessionMicro->>IFrameDispatch: dispatch iframe mutation
IFrameDispatch->>IFrameManager: getParamsByRedirect()
IFrameManager->>AuthorizationServer: navigate iframe to authorize endpoint
AuthorizationServer-->>IFrameManager: redirect to redirectUri
IFrameManager-->>IFrameDispatch: extract params from redirect
IFrameDispatch-->>SessionMicro: return extracted params
SessionMicro-->>OidcClient: SessionCheckSuccess (empty claims)
OidcClient-->>App: {error?: undefined, message?: undefined}
sequenceDiagram
participant App
participant OidcClient as oidc() client
participant SessionMicro as sessionCheckIdTokenµ
participant StorageClient
participant IFrameDispatch
participant JWTValidator
participant AuthorizationServer as IdP
App->>OidcClient: session.check({responseType: 'id_token'})
OidcClient->>SessionMicro: call with generated nonce + state
SessionMicro->>StorageClient: read stored idToken for hint
StorageClient-->>SessionMicro: idToken (or null)
SessionMicro->>SessionMicro: build authorize URL with nonce, state
SessionMicro->>IFrameDispatch: dispatch iframe mutation
IFrameDispatch->>AuthorizationServer: navigate iframe with prompt=none
AuthorizationServer-->>IFrameDispatch: redirect with id_token hash
IFrameDispatch-->>SessionMicro: return params with id_token
SessionMicro->>JWTValidator: decode + validate JWT
JWTValidator->>JWTValidator: verify nonce, state, subject match
JWTValidator-->>SessionMicro: decoded claims
SessionMicro-->>OidcClient: SessionCheckSuccess {claims}
OidcClient-->>App: {error?: undefined, claims: {...}}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
View your CI Pipeline Execution ↗ for commit 98f1e5c
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗ ☁️ Nx Cloud last updated this comment at |
vatsalparikh
changed the title
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Important
At least one additional CI pipeline execution has run since the conclusion below was written and it may no longer be applicable.
Nx Cloud is proposing a fix for your failed CI:
We corrected two failing e2e tests introduced by this PR: the interaction_required assertion replaces the incorrect login_required expectation to match the real OIDC server's response, and the missing 'Session Check (id_token) with subject' button (along with its click handler passing subject: 'expected-user') was added to the oidc-app to unblock the subject mismatch test. These changes complete the UI and test wiring that the PR's implementation requires.
Tip
✅ We verified this fix by re-running @forgerock/oidc-suites:e2e-ci--src/session.spec.ts.
Suggested Fix changes
diff --git a/e2e/oidc-app/src/ping-am/index.html b/e2e/oidc-app/src/ping-am/index.html
index adeaf8d..3c2b90e 100644
--- a/e2e/oidc-app/src/ping-am/index.html
+++ b/e2e/oidc-app/src/ping-am/index.html
@@ -23,6 +23,7 @@
<button id="revoke">Revoke Token</button>
<button id="session-check-btn">Session Check (none)</button>
<button id="session-check-id-token-btn">Session Check (id_token)</button>
+ <button id="session-check-id-token-subject-btn">Session Check (id_token) with subject</button>
<a href="/ping-am/">Start Over</a>
</div>
<script type="module" src="./main.ts"></script>
diff --git a/e2e/oidc-app/src/utils/oidc-app.ts b/e2e/oidc-app/src/utils/oidc-app.ts
index 3c59584..07e2d4d 100644
--- a/e2e/oidc-app/src/utils/oidc-app.ts
+++ b/e2e/oidc-app/src/utils/oidc-app.ts
@@ -230,6 +230,17 @@ export async function oidcApp({
appEl?.appendChild(el);
});
+ document
+ .getElementById('session-check-id-token-subject-btn')
+ ?.addEventListener('click', async () => {
+ const options: SessionCheckOptions = { responseType: 'id_token', subject: 'expected-user' };
+ const result = await oidcClient.session?.check(options);
+ const appEl = document.getElementById('app');
+ const el = document.createElement('div');
+ el.innerHTML = `<p><strong>Session Check (id_token) with subject:</strong></p><pre id="session-check-id-token-result">${JSON.stringify(result, null, 2)}</pre>`;
+ appEl?.appendChild(el);
+ });
+
if (code && state) {
const response = await oidcClient.token?.exchange(code, state);
if (response) {
diff --git a/e2e/oidc-suites/src/session.spec.ts b/e2e/oidc-suites/src/session.spec.ts
index a6f4a2a..1ba8b43 100644
--- a/e2e/oidc-suites/src/session.spec.ts
+++ b/e2e/oidc-suites/src/session.spec.ts
@@ -89,7 +89,7 @@ test.describe('session.check() tests', () => {
expect(resultText).toContain('"error": "login_required"');
});
- test('session check (none) fails with login_required when no session exists', async ({
+ test('session check (none) fails with interaction_required when no session exists', async ({
page,
}) => {
const { navigate } = asyncEvents(page);
@@ -99,7 +99,7 @@ test.describe('session.check() tests', () => {
await page.getByRole('button', { name: 'Session Check (none)', exact: true }).click();
await expect(page.locator('#session-check-result')).not.toBeEmpty();
const resultText = await page.locator('#session-check-result').textContent();
- expect(resultText).toContain('"error": "login_required"');
+ expect(resultText).toContain('"error": "interaction_required"');
});
test('session check (id_token) succeeds with valid JWT in hash', async ({ page }) => {
@@ -128,7 +128,7 @@ test.describe('session.check() tests', () => {
}
});
- await page.getByRole('button', { name: 'Session Check (id_token)' }).click();
+ await page.getByRole('button', { name: 'Session Check (id_token)', exact: true }).click();
await expect(page.locator('#session-check-id-token-result')).not.toBeEmpty();
const resultText = await page.locator('#session-check-id-token-result').textContent();
expect(resultText).toContain('"claims"');
@@ -158,7 +158,7 @@ test.describe('session.check() tests', () => {
}
});
- await page.getByRole('button', { name: 'Session Check (id_token)' }).click();
+ await page.getByRole('button', { name: 'Session Check (id_token)', exact: true }).click();
await expect(page.locator('#session-check-id-token-result')).not.toBeEmpty();
const resultText = await page.locator('#session-check-id-token-result').textContent();
expect(resultText).toContain('"error": "login_required"');
@@ -219,7 +219,7 @@ test.describe('session.check() tests', () => {
}
});
- await page.getByRole('button', { name: 'Session Check (id_token)' }).click();
+ await page.getByRole('button', { name: 'Session Check (id_token)', exact: true }).click();
await expect(page.locator('#session-check-id-token-result')).not.toBeEmpty();
const resultText = await page.locator('#session-check-id-token-result').textContent();
expect(resultText).toContain('"error": "nonce_mismatch"');
Or Apply changes locally with:
npx nx-cloud apply-locally CLoH-jTIZ
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
@forgerock/davinci-client
@forgerock/device-client
@forgerock/journey-client
@forgerock/oidc-client
@forgerock/protect
@forgerock/sdk-types
@forgerock/sdk-utilities
@forgerock/iframe-manager
@forgerock/sdk-logger
@forgerock/sdk-oidc
@forgerock/sdk-request-middleware
@forgerock/storage
commit: |
Codecov Report❌ Patch coverage is ❌ Your project status has failed because the head coverage (21.15%) is below the target coverage (40.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #682 +/- ##
==========================================
+ Coverage 18.07% 21.15% +3.07%
==========================================
Files 155 160 +5
Lines 24398 25115 +717
Branches 1203 1427 +224
==========================================
+ Hits 4410 5313 +903
+ Misses 19988 19802 -186
🚀 New features to boost your workflow:
|
|
Deployed 3246c6a to https://ForgeRock.github.io/ping-javascript-sdk/pr-682/3246c6afcedba7e2b6e13ceefaa16f3bc4d8c8cd branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔺 @forgerock/iframe-manager - 3.2 KB (+0.8 KB, +32.4%) 🆕 New Packages🆕 @forgerock/device-client - 10.0 KB (new) ➖ No Changes➖ @forgerock/protect - 144.6 KB 14 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
vatsalparikh
requested review from
SteinGabriel,
ancheetah,
cerebrl and
ryanbas21
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
e2e/oidc-suites/src/session.spec.ts (1)
15-30: 💤 Low valueConsider base64url-encoding the fake signature for structural correctness.
The
makeFakeJwthelper uses a plain string"fakesignature"for the third segment. While this may work when the SDK only validates the payload (nonce/sub/iat), a structurally correct JWT should have a base64url-encoded signature segment. Consider encoding it for better test fidelity.♻️ Proposed refinement
const body = btoa(JSON.stringify(payload)) .replace(/\+/g, '-') .replace(/\//g, '_') .replace(/=/g, ''); - return `${header}.${body}.fakesignature`; + const signature = btoa('fake-signature-bytes') + .replace(/\+/g, '-') + .replace(/\//g, '_') + .replace(/=/g, ''); + return `${header}.${body}.${signature}`; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/oidc-suites/src/session.spec.ts` around lines 15 - 30, The fake JWT's signature segment is a plain string; update the makeFakeJwt helper to produce a structurally correct base64url-encoded signature instead of "fakesignature". Locate the makeFakeJwt function and replace the static third segment with a base64url-encoded value (e.g., encode a constant like "fakesignature" using the same btoa + replace(/\+/g,'-')/replace(/\//g,'_')/replace(/=/g,'') pattern used for header and body) so the JWT has three properly encoded segments.packages/oidc-client/src/lib/session.micros.ts (2)
25-39: ⚡ Quick winReconsider the error
typefor storage read failures.The error mapping uses
type: 'argument_error'for storage access failures. Storage read failures are typically infrastructure or I/O issues, not argument validation problems. Consider usingtype: 'unknown_error'or a more appropriate type that reflects the nature of storage access failures.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/oidc-client/src/lib/session.micros.ts` around lines 25 - 39, The GenericError returned by readStoredIdTokenµ currently uses type: 'argument_error' which is misleading for storage/I/O failures; update the catch mapper in readStoredIdTokenµ to use a more appropriate error type such as 'unknown_error' (or an I/O/infrastructure-specific type) so storageClient.get() failures are classified correctly, keeping the same error and message fields; ensure the change is applied to the catch block that constructs the GenericError (referencing readStoredIdTokenµ, StorageClient<OauthTokens>, and GenericError).
206-235: ⚡ Quick winConsider validating
redirectUripresence in id_token mode.While id_token mode resolves by detecting the
id_tokenparameter rather than matching the redirect URI, theredirectUriis still included in the authorization request (line 161) and must be valid. Consider adding an early validation check similar to the one insessionCheckNoneµ(lines 186-192) to fail fast ifredirectUriis missing, preventing a confusing AS error or timeout.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/oidc-client/src/lib/session.micros.ts` around lines 206 - 235, sessionCheckIdTokenµ currently proceeds to build the id_token URL without ensuring a redirectUri is present, which can cause confusing AS errors; add the same early validation used in sessionCheckNoneµ to fail fast: before calling buildIdTokenUrl (or immediately after entering sessionCheckIdTokenµ) check for a valid redirectUri (e.g. options?.redirectUri or the config field your code expects) and throw/return a GenericError if missing, mirroring the validation logic from sessionCheckNoneµ so callers get a clear, early error instead of an AS timeout.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/oidc-app/src/utils/oidc-app.ts`:
- Around line 216-231: Replace the use of innerHTML in the session check button
handlers with safe DOM text insertion: instead of setting el.innerHTML use
createElement/textContent to build the nodes and set the JSON string into a text
node (or set el.textContent / the pre element's textContent) so untrusted values
from oidcClient.session?.check() are not interpreted as HTML; update both the
handler for 'session-check-btn' (that currently creates a div and sets innerHTML
with "Session Check (none)" and the JSON) and the handler for
'session-check-id-token-btn' (that sets innerHTML with "Session Check
(id_token)" and the JSON) to construct elements and assign textContent for the
JSON output before appending to the app element.
In `@packages/oidc-client/src/lib/oidc.api.ts`:
- Around line 211-219: The code unsafely casts
URL(req.url).searchParams.get('redirect_uri') to string when calling
iFrameManager().getParamsByRedirect; add a runtime null-check for the
redirect_uri value (derived from URL(req.url).searchParams.get('redirect_uri'))
before invoking iFrameManager().getParamsByRedirect (the resolveOnRedirectUri
parameter), and if it's missing handle it explicitly (throw an error, return a
failure response, or log and reject) so the call only receives a guaranteed
string; update the surrounding code path that builds/consumes redirect_uri
(e.g., buildNoneUrl usages) to maintain type safety and avoid the direct `as
string` cast.
---
Nitpick comments:
In `@e2e/oidc-suites/src/session.spec.ts`:
- Around line 15-30: The fake JWT's signature segment is a plain string; update
the makeFakeJwt helper to produce a structurally correct base64url-encoded
signature instead of "fakesignature". Locate the makeFakeJwt function and
replace the static third segment with a base64url-encoded value (e.g., encode a
constant like "fakesignature" using the same btoa +
replace(/\+/g,'-')/replace(/\//g,'_')/replace(/=/g,'') pattern used for header
and body) so the JWT has three properly encoded segments.
In `@packages/oidc-client/src/lib/session.micros.ts`:
- Around line 25-39: The GenericError returned by readStoredIdTokenµ currently
uses type: 'argument_error' which is misleading for storage/I/O failures; update
the catch mapper in readStoredIdTokenµ to use a more appropriate error type such
as 'unknown_error' (or an I/O/infrastructure-specific type) so
storageClient.get() failures are classified correctly, keeping the same error
and message fields; ensure the change is applied to the catch block that
constructs the GenericError (referencing readStoredIdTokenµ,
StorageClient<OauthTokens>, and GenericError).
- Around line 206-235: sessionCheckIdTokenµ currently proceeds to build the
id_token URL without ensuring a redirectUri is present, which can cause
confusing AS errors; add the same early validation used in sessionCheckNoneµ to
fail fast: before calling buildIdTokenUrl (or immediately after entering
sessionCheckIdTokenµ) check for a valid redirectUri (e.g. options?.redirectUri
or the config field your code expects) and throw/return a GenericError if
missing, mirroring the validation logic from sessionCheckNoneµ so callers get a
clear, early error instead of an AS timeout.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f1fa359c-8961-4f12-9227-9e66f943afc1
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
.changeset/brave-foxes-dance.mde2e/oidc-app/src/ping-am/index.htmle2e/oidc-app/src/ping-one/index.htmle2e/oidc-app/src/utils/oidc-app.tse2e/oidc-suites/src/session.spec.tse2e/oidc-suites/src/utils/login.tspackages/oidc-client/api-report/oidc-client.api.mdpackages/oidc-client/api-report/oidc-client.types.api.mdpackages/oidc-client/package.jsonpackages/oidc-client/src/lib/client.store.test.tspackages/oidc-client/src/lib/client.store.tspackages/oidc-client/src/lib/oidc.api.tspackages/oidc-client/src/lib/session.micros.test.tspackages/oidc-client/src/lib/session.micros.tspackages/oidc-client/src/lib/session.types.tspackages/oidc-client/src/types.tspackages/sdk-effects/iframe-manager/src/lib/iframe-manager.effects.test.tspackages/sdk-effects/iframe-manager/src/lib/iframe-manager.effects.tspnpm-workspace.yaml
| document.getElementById('session-check-btn')?.addEventListener('click', async () => { | ||
| const result = await oidcClient.session?.check(); | ||
| const appEl = document.getElementById('app'); | ||
| const el = document.createElement('div'); | ||
| el.innerHTML = `<p><strong>Session Check (none):</strong></p><pre id="session-check-result">${JSON.stringify(result, null, 2)}</pre>`; | ||
| appEl?.appendChild(el); | ||
| }); | ||
|
|
||
| document.getElementById('session-check-id-token-btn')?.addEventListener('click', async () => { | ||
| const options: SessionCheckOptions = { responseType: 'id_token' }; | ||
| const result = await oidcClient.session?.check(options); | ||
| const appEl = document.getElementById('app'); | ||
| const el = document.createElement('div'); | ||
| el.innerHTML = `<p><strong>Session Check (id_token):</strong></p><pre id="session-check-id-token-result">${JSON.stringify(result, null, 2)}</pre>`; | ||
| appEl?.appendChild(el); | ||
| }); |
There was a problem hiding this comment.
Use textContent instead of innerHTML to prevent potential XSS.
The session check result handlers set innerHTML with JSON.stringify(result). While the result comes from the OIDC client (not direct user input) and JSON.stringify provides some escaping, it's not complete XSS protection. Claims or error descriptions from the authorization server could theoretically contain malicious content. Best practice is to use textContent for untrusted data.
🛡️ Proposed fix for session check handlers
document.getElementById('session-check-btn')?.addEventListener('click', async () => {
const result = await oidcClient.session?.check();
const appEl = document.getElementById('app');
const el = document.createElement('div');
- el.innerHTML = `<p><strong>Session Check (none):</strong></p><pre id="session-check-result">${JSON.stringify(result, null, 2)}</pre>`;
+ const pre = document.createElement('pre');
+ pre.id = 'session-check-result';
+ pre.textContent = JSON.stringify(result, null, 2);
+ el.innerHTML = '<p><strong>Session Check (none):</strong></p>';
+ el.appendChild(pre);
appEl?.appendChild(el);
});
document.getElementById('session-check-id-token-btn')?.addEventListener('click', async () => {
const options: SessionCheckOptions = { responseType: 'id_token' };
const result = await oidcClient.session?.check(options);
const appEl = document.getElementById('app');
const el = document.createElement('div');
- el.innerHTML = `<p><strong>Session Check (id_token):</strong></p><pre id="session-check-id-token-result">${JSON.stringify(result, null, 2)}</pre>`;
+ const pre = document.createElement('pre');
+ pre.id = 'session-check-id-token-result';
+ pre.textContent = JSON.stringify(result, null, 2);
+ el.innerHTML = '<p><strong>Session Check (id_token):</strong></p>';
+ el.appendChild(pre);
appEl?.appendChild(el);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| document.getElementById('session-check-btn')?.addEventListener('click', async () => { | |
| const result = await oidcClient.session?.check(); | |
| const appEl = document.getElementById('app'); | |
| const el = document.createElement('div'); | |
| el.innerHTML = `<p><strong>Session Check (none):</strong></p><pre id="session-check-result">${JSON.stringify(result, null, 2)}</pre>`; | |
| appEl?.appendChild(el); | |
| }); | |
| document.getElementById('session-check-id-token-btn')?.addEventListener('click', async () => { | |
| const options: SessionCheckOptions = { responseType: 'id_token' }; | |
| const result = await oidcClient.session?.check(options); | |
| const appEl = document.getElementById('app'); | |
| const el = document.createElement('div'); | |
| el.innerHTML = `<p><strong>Session Check (id_token):</strong></p><pre id="session-check-id-token-result">${JSON.stringify(result, null, 2)}</pre>`; | |
| appEl?.appendChild(el); | |
| }); | |
| document.getElementById('session-check-btn')?.addEventListener('click', async () => { | |
| const result = await oidcClient.session?.check(); | |
| const appEl = document.getElementById('app'); | |
| const el = document.createElement('div'); | |
| const pre = document.createElement('pre'); | |
| pre.id = 'session-check-result'; | |
| pre.textContent = JSON.stringify(result, null, 2); | |
| el.innerHTML = '<p><strong>Session Check (none):</strong></p>'; | |
| el.appendChild(pre); | |
| appEl?.appendChild(el); | |
| }); | |
| document.getElementById('session-check-id-token-btn')?.addEventListener('click', async () => { | |
| const options: SessionCheckOptions = { responseType: 'id_token' }; | |
| const result = await oidcClient.session?.check(options); | |
| const appEl = document.getElementById('app'); | |
| const el = document.createElement('div'); | |
| const pre = document.createElement('pre'); | |
| pre.id = 'session-check-id-token-result'; | |
| pre.textContent = JSON.stringify(result, null, 2); | |
| el.innerHTML = '<p><strong>Session Check (id_token):</strong></p>'; | |
| el.appendChild(pre); | |
| appEl?.appendChild(el); | |
| }); |
🧰 Tools
🪛 ast-grep (0.43.0)
[warning] 219-219: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: el.innerHTML = <p><strong>Session Check (none):</strong></p><pre id="session-check-result">${JSON.stringify(result, null, 2)}</pre>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation
(dom-content-modification)
[warning] 228-228: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: el.innerHTML = <p><strong>Session Check (id_token):</strong></p><pre id="session-check-id-token-result">${JSON.stringify(result, null, 2)}</pre>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation
(dom-content-modification)
[warning] 219-219: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: el.innerHTML = <p><strong>Session Check (none):</strong></p><pre id="session-check-result">${JSON.stringify(result, null, 2)}</pre>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation
(unsafe-html-content-assignment)
[warning] 228-228: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: el.innerHTML = <p><strong>Session Check (id_token):</strong></p><pre id="session-check-id-token-result">${JSON.stringify(result, null, 2)}</pre>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation
(unsafe-html-content-assignment)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/oidc-app/src/utils/oidc-app.ts` around lines 216 - 231, Replace the use
of innerHTML in the session check button handlers with safe DOM text insertion:
instead of setting el.innerHTML use createElement/textContent to build the nodes
and set the JSON string into a text node (or set el.textContent / the pre
element's textContent) so untrusted values from oidcClient.session?.check() are
not interpreted as HTML; update both the handler for 'session-check-btn' (that
currently creates a div and sets innerHTML with "Session Check (none)" and the
JSON) and the handler for 'session-check-id-token-btn' (that sets innerHTML with
"Session Check (id_token)" and the JSON) to construct elements and assign
textContent for the JSON output before appending to the app element.
Source: Linters/SAST tools
| : await iFrameManager().getParamsByRedirect({ | ||
| url: req.url, | ||
| resolveOnRedirectUri: new URL(req.url).searchParams.get( | ||
| 'redirect_uri', | ||
| ) as string, | ||
| errorParams, | ||
| successParams: [], | ||
| timeout, | ||
| }); |
There was a problem hiding this comment.
Add runtime validation for redirect_uri parameter.
The cast to string on Line 213-215 is unsafe because searchParams.get() returns string | null. While buildNoneUrl always includes redirect_uri, type safety isn't guaranteed.
🛡️ Proposed fix to add validation
: await iFrameManager().getParamsByRedirect({
url: req.url,
- resolveOnRedirectUri: new URL(req.url).searchParams.get(
- 'redirect_uri',
- ) as string,
+ resolveOnRedirectUri:
+ new URL(req.url).searchParams.get('redirect_uri') ??
+ (() => {
+ throw new Error('redirect_uri missing from session check URL');
+ })(),
errorParams,
successParams: [],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| : await iFrameManager().getParamsByRedirect({ | |
| url: req.url, | |
| resolveOnRedirectUri: new URL(req.url).searchParams.get( | |
| 'redirect_uri', | |
| ) as string, | |
| errorParams, | |
| successParams: [], | |
| timeout, | |
| }); | |
| : await iFrameManager().getParamsByRedirect({ | |
| url: req.url, | |
| resolveOnRedirectUri: | |
| new URL(req.url).searchParams.get('redirect_uri') ?? | |
| (() => { | |
| throw new Error('redirect_uri missing from session check URL'); | |
| })(), | |
| errorParams, | |
| successParams: [], | |
| timeout, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/oidc-client/src/lib/oidc.api.ts` around lines 211 - 219, The code
unsafely casts URL(req.url).searchParams.get('redirect_uri') to string when
calling iFrameManager().getParamsByRedirect; add a runtime null-check for the
redirect_uri value (derived from URL(req.url).searchParams.get('redirect_uri'))
before invoking iFrameManager().getParamsByRedirect (the resolveOnRedirectUri
parameter), and if it's missing handle it explicitly (throw an error, return a
failure response, or log and reject) so the call only receives a guaranteed
string; update the surrounding code path that builds/consumes redirect_uri
(e.g., buildNoneUrl usages) to maintain type safety and avoid the direct `as
string` cast.
| storageClient: StorageClient<OauthTokens>, | ||
| ): Micro.Micro<string | null, GenericError, never> => { | ||
| return Micro.tryPromise({ | ||
| try: async () => { |
There was a problem hiding this comment.
We shouldn't need the async/await here. Let's break this down granularly, handle the get in tryPromise
then map over the tokens && idToken.
psuedocode:
Micro.tryPromise({ try: () => storageClient.get(), catch: failed to get storage client }).pipe(
Micro.map(tokens => tokens && 'idToken' in tokens ? tokens.idToken : null)
Writing it out this way breaks out another issue, we have null as a possible value. We can now consider, do we want null to be success? do we want it to be failure? do we want to lift the predicate out and evaluate it there?
Something to think about - while it doesn't need to be addressed because it's not a concrete solution, breaking out the pieces like this will allow us to compose this and refactor easily. it also allows us to create granular errors for the step in the micro we are at.
| if ('error' in result && result.error) { | ||
| const errData = result.error as { | ||
| data?: { error?: string; message?: string; type?: string }; | ||
| }; |
There was a problem hiding this comment.
Why have this check if we can just handle it in the catch above? seems like we're duplicating error handling? We handle it as a Generic error and then update it as errData? This seems like it can be a singular pipe,
Micro.tryPromise({
try: store-dispatch,
catch: error
}).pipe(
Micro.mapError(fail),
Micro.succeed(success)
);
We can also consider, do we want to "fail" here in session.micro.ts or do we want to allow the failure to be consumed later on in the pipeline? That way it's possible the session check here can be composed and handled uniquely when it needs to be? Again - similarly this is just me thinking in my head, and not something to specifically be addressed but i'm trying to break down my thought process in why I think to write the effectful pipelines as i'm suggesting.
| const redirectUri = options?.redirectUri ?? config.redirectUri; | ||
| // none mode resolves by recognising when the iframe lands on the redirect URI. | ||
| // An empty redirect_uri means the iframe never matches and silently times out instead of failing fast. | ||
| if (!redirectUri) { | ||
| return yield* Micro.fail<GenericError>({ | ||
| error: 'missing_redirect_uri', | ||
| message: 'redirect_uri is required for session check', | ||
| type: 'argument_error', | ||
| }); | ||
| } |
There was a problem hiding this comment.
semantics - but I think it may be nice to handle the error state above at the top of the function so that way we short-circuit before running any code if not necessary.
| const url = buildNoneUrl(wellknown.authorization_endpoint, config, storedIdToken, options); | ||
| log.debug('Session check (none) URL built'); | ||
|
|
||
| yield* dispatchSessionCheckµ(store, url, SessionCheckResponseType.None); | ||
| log.debug('Session check (none) completed successfully'); | ||
|
|
||
| return {} satisfies SessionCheckSuccess; | ||
| }); |
There was a problem hiding this comment.
if we do the above comment - we can create a singular pipeline for this now.
| const storedIdToken = yield* readStoredIdTokenµ(storageClient); | ||
| const { url, nonce, state } = buildIdTokenUrl( | ||
| wellknown.authorization_endpoint, | ||
| config, | ||
| storedIdToken, | ||
| options, | ||
| ); | ||
| log.debug('Session check (id_token) URL built'); | ||
|
|
||
| const iframeParams = yield* dispatchSessionCheckµ(store, url, SessionCheckResponseType.IdToken); | ||
| const claims = yield* validateSessionCheckResponseµ( | ||
| iframeParams, | ||
| state, | ||
| nonce, | ||
| options?.subject, | ||
| ); | ||
| log.debug('Session check (id_token) completed successfully'); | ||
|
|
||
| return { claims } satisfies SessionCheckSuccess; |
There was a problem hiding this comment.
This is fine - i have no issues with it personally. However, I think as a team, we tend to prefer pipe syntax where it's cleaner. I'd ask if you write it in a pipe if you feel it's cleaner or harder to read. and then just weigh that decision. I think this can be done pretty cleanly as a pipe.
cerebrl
left a comment
cerebrl
left a comment
There was a problem hiding this comment.
Left some comments after an initial review. I'll take a deeper look at your .micro.ts file soon.
| /** | ||
| * An object containing methods for OIDC session management | ||
| */ | ||
| session: { |
There was a problem hiding this comment.
I'm not sure how I feel about adding a session property here. Since we are checking the user's current session, I feel like we could move this into the user property. user.logout, for example, terminates the current user session.
| // Both a const object (for runtime value access: SessionCheckResponseType.IdToken) and a type | ||
| // (for annotations: param: SessionCheckResponseType) are declared under the same name. | ||
| // This is the TypeScript const-object + union-type pattern — a tree-shakeable alternative to enums | ||
| // that preserves the string literals ('id_token' | 'none') in the compiled output. | ||
| export const SessionCheckResponseType = { | ||
| IdToken: 'id_token', | ||
| None: 'none', | ||
| } as const; | ||
|
|
||
| export type SessionCheckResponseType = | ||
| (typeof SessionCheckResponseType)[keyof typeof SessionCheckResponseType]; |
There was a problem hiding this comment.
I have to admit, this really confused me for longer than a care to admit. The fact that they are both named the same, but serve different purposes ... the const is actual runtime code, where the type is compile-time only diverges away from our patterns. We don't really want runtime code in our .type.ts files as they should be completely compiled away.
I would be more open to this if we had to derive the type dynamically from something external, or something that is highly volatile or unknown at compile time. Since this is a static config property that is very simple and will likely never change, this feels over-engineered.
This pattern essentially gets reduced down to a string union: 'id_token' | 'none', why don't we just directly write it as such? When the type is directly declared as a union of strings, you get the same amount of type-ahead/autocomplete/spellcheck as a string enum. So, I'm wondering why the additional complexity.
| ? await iFrameManager().getParamsByRedirect({ | ||
| url: req.url, | ||
| successParams: ['id_token'], | ||
| errorParams, | ||
| includeHashParams: true, | ||
| timeout, | ||
| }) | ||
| : await iFrameManager().getParamsByRedirect({ | ||
| url: req.url, | ||
| resolveOnRedirectUri: new URL(req.url).searchParams.get( | ||
| 'redirect_uri', | ||
| ) as string, | ||
| errorParams, | ||
| successParams: [], | ||
| timeout, |
There was a problem hiding this comment.
Since this ternary calls the same function, but with slightly different configs, can we just construct the config within a condition, and call the function once with whatever config is built? By splitting the actual function calls between the condition, it reads more complex than it really is.
| // With resolveOnRedirectUri (response_type=none), success is detected via URI landing so | ||
| // successParams:[] is intentional — but errorParams is still required since errors arrive as query params. |
There was a problem hiding this comment.
I'm not sure I understand these changes. Can elaborate on this case where there are neither success or error params?
4 participants
https://pingidentity.atlassian.net/browse/SDKS-5003
Implementation Decisions
How to test
pnpm nx build oidc-app && pnpm nx serve oidc-apphttp://localhost:8443/ping-ampage and Login (Background)Recording
Screen.Recording.2026-06-09.at.7.32.41.AM.mov
Summary by CodeRabbit